fix(integrations): stop installed runtime configs drifting silently - #107

Claude finished @ubercylon8's task in 2m 12s —— View job

Security review — PR #107

• Gather context (PR diff, CLAUDE.md rules)
• Review scripts/sync_pi_config.py merge logic
• Review integrations/test_integrations_valid.py new guards
• Review integrations/hermes/distribution/distribution.yaml changes
• Review docs/user-guide/runtimes/hermes.md changes
• Post final findings

Scope note: this PR touches only local dev tooling (scripts/sync_pi_config.py), doc-drift test guards, and docs — it does not touch core/gating, core/redaction, core/schema, or any servers/* tool. So most of CLAUDE.md's Critical Rules (gated writes, redaction, findings schema, small-model-safe tool schemas) don't apply to this diff. I reviewed it against the rules that do apply: secrets never leaving the host, and correctness/test coverage of the changed logic.

No secrets or credentials are handled by this code — integrations/pi/mcp.json only carries command/args/transport, no API keys (servers load .env.<platform> themselves at runtime), so the merge in merge_into_existing (scripts/sync_pi_config.py:42-67) never touches secret material. No security issues found.

One correctness/robustness observation (non-blocking, not a vulnerability):

merge_into_existing (scripts/sync_pi_config.py:63) decides "foreign" purely by name not in ours — i.e. anything not in the current template. That correctly protects entries from sibling repos (the bug this PR fixes), but it also means if this repo ever renames or removes a server, the old entry silently becomes indistinguishable from a genuinely foreign one and is preserved forever with a stale command line — the merge has no way to tell "no longer ours" apart from "never was ours." That's a mild, opposite-direction version of the drift problem this PR is fixing (a zombie entry instead of a lost one), just far less likely to bite and not a secrets/gating/redaction issue. Worth a one-line comment or a follow-up if the team wants a "known limitation" note; not worth blocking on.

Everything else checks out:

• The merge preserves foreign entries and refreshes ours (scripts/sync_pi_config.py:42-67), matches the described bug and is covered by scripts/test_sync_pi_config.py:27-45.
• New guards in integrations/test_integrations_valid.py correctly enforce that distribution.yaml's description lists every servers/* platform and that its version tracks pyproject.toml — verified version: 0.3.0 in both files.
• docs/user-guide/runtimes/hermes.md additions are accurate (installed-profile-is-a-copy caveat, re-install step, diff one-liner) and introduce no secret-handling guidance issues.

Note: I couldn't execute uv run pytest / ruff / mypy in this review sandbox (bash tool calls required interactive approval I don't have here), so this is a static review only — the PR description states 1083 tests pass with ruff/mypy clean, which I have no reason to doubt from reading the diff.

Took the review's observation rather than deferring it — 2c8fbba.

It is right that "absent from the template" only solves the drift in one direction: a server we renamed or removed would be indistinguishable from a sibling repo's entry and preserved forever, pointing at a command that no longer exists. A zombie instead of a deletion, but the same silent drift this merge exists to prevent — and this session has already spent a while on exactly that class of bug.

Ownership is now decided by whether an entry references this checkout, not by whether the template still names it. A stale entry of ours is dropped; another repo's is untouched. Verified against the live pi install: f0-library points at the sibling checkout, so it survives and --check reports no change.

Mutation-tested by reverting to name-only ownership — the new test fails as it should. 1084 passing.

Claude finished @ubercylon8's task in 1m 12s —— View job

Security review — PR #107 (re-run on latest push)

• Gather context (PR diff vs origin/main, CLAUDE.md rules)
• Review scripts/sync_pi_config.py merge logic
• Review integrations/test_integrations_valid.py new guards
• Review integrations/hermes/distribution/distribution.yaml changes
• Review docs/user-guide/runtimes/hermes.md changes
• Post final findings

Scope note (unchanged from the last pass on this PR): this diff touches only local dev tooling (scripts/sync_pi_config.py), doc-drift test guards, and docs — no core/gating, core/redaction, core/schema, or servers/* tool code. Most of CLAUDE.md's Critical Rules (gated writes, redaction, findings schema, small-model-safe tool schemas) don't apply here. Reviewed against what does apply: secrets-never-leave-the-host, and correctness/test coverage of the changed logic.

No secrets or credentials are handled by this code. integrations/pi/mcp.json and the merge in merge_into_existing (scripts/sync_pi_config.py:42-74) only ever touch command/args/transport — no API keys (servers load .env.<platform> themselves at runtime). No security issues found.

Follow-up on the earlier finding — now fixed. My previous pass on this PR flagged that deciding "foreign" purely by name not in ours would let a renamed/removed server become a permanent zombie entry (indistinguishable from a genuinely foreign one, preserved forever with a stale command line). Commit 2c8fbba addresses this directly: ownership is now decided by whether an entry's JSON references this checkout's absolute path (marker = str(REPO), scripts/sync_pi_config.py:65-70), not just by name. A stale entry of ours (name gone from the template, but still pointing at str(sync.REPO)) is now dropped instead of preserved; a sibling repo's entry (different path) still survives. This is covered by a new test, test_sync_drops_a_server_this_repo_no_longer_ships (scripts/test_sync_pi_config.py:48-65), alongside the original test_sync_preserves_servers_this_repo_does_not_own (scripts/test_sync_pi_config.py:29-45). Both directions of the drift are now exercised.

Minor, non-blocking observation: the marker match is a plain substring check on json.dumps(entry) against str(REPO). If a live install's entry was rendered from a different absolute path to the same checkout (e.g. via a symlink, a mount re-mapped, or the repo moved), the string wouldn't match and a stale entry could revert to being treated as foreign again — i.e. the fix's correctness is tied to the checkout's path staying stable between renders. This is a narrow edge case, not a secrets/gating/redaction issue, and not worth blocking on.

New drift guards checked out:

• test_distribution_manifest_names_every_platform_it_ships (integrations/test_integrations_valid.py) correctly derives the platform-name set from servers/* dirs (projectachilles-mcp / projectachilles-actions-mcp both collapse to projectachilles, handled explicitly) and asserts each appears in the manifest description. Verified against the actual servers/ listing — all 8 platforms present in the updated description.
• test_distribution_version_tracks_the_repo_version correctly ties distribution.yaml's version to pyproject.toml's project.version (0.3.0 in both, confirmed).
• docs/user-guide/runtimes/hermes.md additions are accurate: installed-profile-is-a-copy caveat, re-install step, and the diff-based check-without-installing one-liner. No secret-handling guidance issues.

Nothing else stood out. I was not able to execute uv run pytest / ruff / mypy in this review sandbox (no live shell access here), so this remains a static review — the PR states 1083 tests passing with ruff/mypy clean, and I have no reason to doubt that from reading the diff.